test(desktop): pin the About reader's missing-provider throw - #4905
Conversation
`AppUpdateAboutProjectionConsumer` throws when mounted outside `AppUpdateProvider`; the scope suite covered only the mounted path. Add the `assert.throws` render case, and trim the `AboutUpdateStatusRow` doc comment to the sentence the next editor needs (the ownership argument now lives in the PR body).
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
No substantiated findings at 8269d2d. The new test mounts the actual AppUpdateAboutProjectionConsumer without its provider, verifies the missing-provider error and ensures the projection callback never runs (app-update-provider-scope.test.ts:260; app-update-projection-context.ts:34). The other change is comment-only.
The full build:test succeeds after applying the repository's required dependency patches, and all four provider-scope tests pass. A production-only load-hook mutation replacing the consumer's throw with a quiet return makes the new test fail with Missing expected exception, confirming it distinguishes the prohibited fallback. No full Desktop suite or live Electron UI run was performed. No blocker found within this test-focused scope; this is not an approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Automated review by OpenAI Codex, operated by me2seeks, at 8269d2d37e08b62c2aa8541d945e3104149c59ea. This is an automated technical assessment, not an independent human review. Approval is submitted at the operator's explicit direction.
No actionable findings. The new test renders the actual About projection consumer outside its provider and requires both the missing-provider throw and zero projection callback executions. The consumer currently throws before invoking that callback; a quiet fallback fails assert.throws. The only production-file edit trims a comment.
- Optimal for the actual problem: Yes; the change addresses the defect at its existing owner.
- Production code that can be deleted: None identified.
- Low-quality tests that can be deleted or replaced: None identified.
- Deeper refactor: No deeper refactor is needed.
- Ready to merge: Content is ready; merge remains conditional on current required checks and conflict state.
- Residual risks / verification: Checked the consumer, fake renderer, per-test cleanup, and exact diff; required hosted test passed. Existing independent automated review also reports a throw-to-null mutation failure; that mutation was not rerun locally. No protected-area effect identified in this diff.
3 participants
Summary
Follow-up to #4498, taking the two non-blocking P3s from the re-review; the commit carrying them was pushed after the merge, so it lands here.
AppUpdateAboutProjectionConsumerthrows when mounted outsideAppUpdateProvider, butapp-update-provider-scope.test.tscovered only the mounted path. Add theassert.throwsrender case (React'sactrethrows the uncaught render error) and assert the reader's callback never ran.AboutUpdateStatusRowdoc comment to the sentence the next editor needs, that it is a component because the action guard is a hook; the ownership argument lives in refactor(desktop): move App Update controller below AppShell #4498's body.Refs #4582
Verification
test:dist2312/2312,@maka/desktoptypecheck, lint, format:checkcheck:renderer-architecture --base upstream/main,check:app-shell-hooks, Astryx inventory, Knip (apps/desktop), ASF headers,git diff --checkreturn null(checked locally before pushing)AI use
Select exactly one:
Tool(s) and scope: Claude Code wrote the test case and the comment trim and ran the verification; the human contributor reviewed the work and chose to submit it.
Checklist
Does this PR entail a change in behavior?